Better TS #302

lishaduck · 2024-11-06T03:02:45Z

Follow up to #301.

Works on TSESlint-based safety.
Probably broke some stuff with error handling.

Please review closely,¹ some of these changes might be better served with ignores. Then again, ignores are evil, so... I just fixed them all and hoped for the best.

Oh, and I added more JSDocs for literally everything! Yay!

EDIT: 🤣 ↩

I needed better types for `parse-elm`, and ended up with this beauty. Disclaimer: `NestedEntrypoints` type courtesy of Copilot.

You can throw anything. Now, we only have to deal with Errors. Probably a slight perf hit, but likely only on the unhappy path, so it's not as critical.

Uhhhh...... Effect is heavy, and TS didn't like the way message passing was done... And, so I uh... wrote a result type. And then I made it generic, so voilà! The only issue is that TS can't type pipes, so I'd need to make a fluent interface or write a ton of overloads. I opted to just force normal functions.

socket-security · 2024-11-06T17:52:11Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@package-json/[email protected]	None	`0`	17.8 kB	kwaa

View full report↗︎

Address review comments from jfmengels#301.

@jfmengels

@jfmengels, without more context for the intent of this code, I'm not sure where the floating promises were intentional. I've marked them as ignored so that we don't introduce more issues, but I'd appreciate any insight you can provide on further steps.

lishaduck · 2024-11-08T17:09:36Z

@jfmengels, ping pong 🏓
:)

jfmengels · 2024-11-08T21:45:13Z

lib/utils.js

+ * @param {unknown} error
+ */
+function intoError(error) {
+  return error instanceof Error ? error : new Error(String(error));


I'm curious, did you notice places where we (potentially) wrapped an exception into multiple Errors? (nested I mean)

Related: did you notice places where we throw non-Errors? 🤔
(Note, I haven't read the rest of the commits yet)

I'm curious, did you notice places where we (potentially) wrapped an exception into multiple Errors? (nested I mean)

Nope.

Related: did you notice places where we throw non-Errors? 🤔 (Note, I haven't read the rest of the commits yet)

Yeah, wenode-elm-compiler throws strings in a few places.

jfmengels · 2024-11-08T21:52:14Z

lib/elm-app-worker.js

 const ElmCommunication = require('./elm-communication');
 const loadCompiledElmApp = require('./load-compiled-app');
 const ResultCache = require('./result-cache');

+/**
+ * @type {WorkerThreads<WorkerData>}
+ */


Should the type annotation not ideally be placed at the definition of workerThreads? (That at least sounds a lot more intuitive to me)

I tried that, but it doesn't like it.

jfmengels · 2024-11-08T21:57:52Z

lib/sync-get-worker.js

@@ -15,9 +16,13 @@ if (parentPort) {
  parentPort.on('message', async (url) => {
    try {
      const response = await getBody(url);
-      requestPort.postMessage(response);
+      requestPort.postMessage(
+        /** @satisfies {PortResponse} */ (result.succeed(response))


Let's make it look even more like Elm!

const Result = require('./result'); Result.succeed(response)

Maybe even call the constructors Ok and Err (or Fail, because I can see it could be confusing with Error/exceptions)

jfmengels · 2024-11-08T22:03:25Z

lib/options.js

-      if (flag.alias !== undefined) {
-        object[flag.name] = flag.alias;
-      }
+  const args = /** @type {ParsedArgs} */ (


What's the difference between

const args = /** @type {ParsedArgs} */ ...

and

/** @type {ParsedArgs} */ const args = ...

Are the two different from TS' point of view?
(Or is this just a styling change?)

The first one is a cast, the second one is a type annotation. I don't recall why this needed a cast.

jfmengels · 2024-11-08T22:05:09Z

lib/remote-template.js

@@ -338,7 +340,10 @@ async function makeGitHubApiRequest(options, url, handleNotFound) {
    // TODO(@lishaduck) [engine:node@>=21]: We can use `fetch` now.
    const response = await got(url, parameters);

-    return response.body;
+    /** @type {{body: unknown}} */


Seeing this over and over, I feel like TS should try to understand type annotations on a return statement like

/** @type {{body: unknown}} */ return response.body;

It's actually a ts-eslint issue: typescript-eslint/typescript-eslint#1682. It can't read JSDoc casts b/c they're technically just comments, so the type queries are wrong. no-unsafe-return bans returning any, which is good, but can't read immediate casts, it needs an intermediate variable. In normal TS, you can just use as.

jfmengels · 2024-11-08T22:07:42Z

lib/template-dependencies.js

    );
+
+    return /** @type {ApplicationDependencies} */ (dependencies);


Just so I understand, the /** @type {unknown} */ is for correctly annotating the value of dependencies, and the type annotation here is to cast?

Or are they both for casting? In which case, why not cast only once?

When using JSON.parse (& other lib functions), you generally want to cast to unknown, then to another type (the as unknown as pattern), because they don't default to unknown for back compat.
I'd guess it's not a direct return b/c of no-unsafe-returns.

jfmengels · 2024-11-08T22:15:21Z

lib/project-dependencies.js

@@ -29,7 +30,8 @@ async function collect(options, elmJson, elmVersion) {
  let hasDependenciesThatCouldNotBeDownloaded = false;

  const projectDepsPromises = Object.entries(dependenciesEntries).map(
-    async ([name, constraint]) => {
+    async ([pkgname, constraint]) => {
+      const name = /** @type {PackageName} */ (pkgname);


(Mostly out of curiosity) Could this type potentially be inferred by typing dependenciesEntries instead?

It is typed, but because of excess properties, ts can't infer it: FAQ § Excess Properties Are OK.

jfmengels · 2024-11-08T22:27:47Z

test/jest-helpers/cli.js

@@ -12,15 +12,16 @@ expect.extend({toMatchFile});

 /**
 * @param {string} args
- * @param {Options} [options=undefined]
+ * @param {Options} [options]
+ * @returns


Is the type not missing here? (same in the rest of the file)

With options, the [] syntax defaults to undefined.
With the @returns, I must've just missed it.

jfmengels · 2024-11-08T22:37:22Z

lib/runner.js

@@ -139,7 +139,7 @@ async function initializeApp(options, elmModulePath, reviewElmJson, appHash) {
     * @returns {never}
     */
    (errors) => {
-      Report.report(options, {errors});
+      void Report.report(options, {errors});


I'm not sure I knew that worker.terminate() returned a Promise back when I wrote it, but I think all the floating promises in this commit make sense to keep as floating (for concurrency purposes, etc.).

The only one I'd be worried about it potentially this one, but so far it has always worked well so... I think it's fine to keep as is 🤷

jfmengels · 2024-11-08T22:42:21Z

Great work!

Btw, thank you for your nice commit etiquette for your PRs. It's easy to review each commit (except maybe for 3965c34 😛 , although it was not that bad in practice)

lishaduck added 8 commits November 5, 2024 13:52

refactor: fancy-dancy elm-js stuff

8fbe6f6

I needed better types for `parse-elm`, and ended up with this beauty. Disclaimer: `NestedEntrypoints` type courtesy of Copilot.

refactor: better, safer PackageName stuff

21d0087

refactor: safer errors

9c153eb

You can throw anything. Now, we only have to deal with Errors. Probably a slight perf hit, but likely only on the unhappy path, so it's not as critical.

refactor: safer workers

cdd0c4b

chore: enable no-unsafe-call

b19e9d6

chore: enable no-unsafe-return

c6d5eb9

chore: update eslint more

4a54b16

lishaduck force-pushed the better-ts branch from b7a4d34 to 0d056a5 Compare November 6, 2024 17:53

docs: update jsdocs

3965c34

Address review comments from jfmengels#301.

lishaduck force-pushed the better-ts branch from 0d056a5 to 3965c34 Compare November 6, 2024 18:18

lishaduck marked this pull request as ready for review November 6, 2024 18:35

lishaduck added 2 commits November 6, 2024 16:28

fix: unbound method

b1c524a

fix: enable type-checked rules in eslintrc

6ca8dfb

lishaduck force-pushed the better-ts branch from 3189669 to 6ca8dfb Compare November 6, 2024 22:28

lishaduck mentioned this pull request Nov 6, 2024

ESLint Improvements #172

Open

12 tasks

lishaduck force-pushed the better-ts branch from d966c6f to fb9e058 Compare November 6, 2024 22:59

fix: errors caught by promise rules

d594fca

lishaduck force-pushed the better-ts branch from fb9e058 to d594fca Compare November 6, 2024 23:03

lishaduck added 3 commits November 6, 2024 17:12

chore: enable prefer-promise-reject-errors

b5e9388

chore: enable no-misused-promises

a991e03

jfmengels reviewed Nov 8, 2024

View reviewed changes

jfmengels merged commit a991e03 into jfmengels:main Nov 8, 2024
3 checks passed

lishaduck deleted the better-ts branch November 8, 2024 23:04

lishaduck mentioned this pull request Nov 10, 2024

Even Better TS #307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better TS #302

Better TS #302

lishaduck commented Nov 6, 2024 •

edited

Loading

socket-security bot commented Nov 6, 2024 •

edited

Loading

lishaduck commented Nov 8, 2024

jfmengels Nov 8, 2024 •

edited

Loading

lishaduck Nov 8, 2024 •

edited

Loading

jfmengels Nov 8, 2024

lishaduck Nov 8, 2024

jfmengels Nov 8, 2024

jfmengels Nov 8, 2024

lishaduck Nov 8, 2024

jfmengels Nov 8, 2024

lishaduck Nov 8, 2024

jfmengels Nov 8, 2024

lishaduck Nov 8, 2024

jfmengels Nov 8, 2024

lishaduck Nov 8, 2024

jfmengels Nov 8, 2024 •

edited

Loading

lishaduck Nov 8, 2024

jfmengels Nov 8, 2024

jfmengels commented Nov 8, 2024

		);

		return /** @type {ApplicationDependencies} */ (dependencies);

Better TS #302

Better TS #302

Conversation

lishaduck commented Nov 6, 2024 • edited Loading

Footnotes

socket-security bot commented Nov 6, 2024 • edited Loading

lishaduck commented Nov 8, 2024

jfmengels Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

lishaduck Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfmengels Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfmengels commented Nov 8, 2024

lishaduck commented Nov 6, 2024 •

edited

Loading

socket-security bot commented Nov 6, 2024 •

edited

Loading

jfmengels Nov 8, 2024 •

edited

Loading

lishaduck Nov 8, 2024 •

edited

Loading

jfmengels Nov 8, 2024 •

edited

Loading